Fix drop() method to handle quoted column names consistently #1242
Conversation
- Strip quotes from column names in drop() method
- Maintains consistency with other DataFrame operations
- Both drop('col') and drop('col') now work
Fixes apache#1212
|
Hi thanks for the PR.
In the context of CSV capitalised col header, I just tested out Of course, it'd be great if one can opt to use double quote or without to parse capitalised col header for DataFrame operations like Let me know if I misunderstood the context. |
| Returns: | ||
| DataFrame with those columns removed in the projection. | ||
| """ |
There was a problem hiding this comment.
Thank you for the PR. This looks good. One request: Would you mind updating the docstring to specify that column case is respected and does not need double quotes like other operations such as select? You can also specify that leading and trailing " are allowable as well.
There was a problem hiding this comment.
I've updated the docstring.
Based on the comment from @HeWhoHeWho should we open another issue for select(), drop(), sort() or should we expand the scope of this issue?
There was a problem hiding this comment.
drop is unique in that it only takes in column names. The others differ in that they take expressions. We have some syntactic sugar around them to allow passing in strings that get turned into expressions. From the perspective of datafusion, requiring the double quotes for select and sort is the expected behavior. It is also included in the online documentation with a call out box to emphasize it. So I think we are okay with just this PR as it is.
If people want the behavior of select and sort changed then we should consider adding an upstream change to the datafusion repo with a configuration setting to do this. I tested with the datafusion.sql_parser.enable_ident_normalization but that didn't seem to make an impact.
There was a problem hiding this comment.
If people want the behavior of
selectandsortchanged then we should consider adding an upstream change to thedatafusionrepo with a configuration setting to do this. I tested with thedatafusion.sql_parser.enable_ident_normalizationbut that didn't seem to make an impact.
Say we go by - if Expression is used, then force the usage of double quote, otherwise allow select('col') or select('"col"'), same goes for sort.
Would this be a big change on upstream at datafusion repo?
- Document that column names are case-sensitive and don't require quotes - Clarify that both quoted and unquoted column names are accepted - Add examples showing both 'col' and 'col' syntax work - Note difference from select() operation behavior
|
@H0TB0X420 it looks like you have some |
|
I'm running CI now! It looks like there were a couple of changes that we needed and I pushed. First there were whitespace errors and some lines that were too long. We use The second was that the documentation did not render correctly. RestructuredText (rst) can be fickle if you haven't used it before. It needs double back ticks rather than single ticks to indicate something should render as monospaced variables. Also in order to get the python example to work the line before it needs to end with a double colon Thank you again for the PR! Assuming CI passes I'll merge this. |
Rationale: CSV files with capitalized headers require quotes in
select()operations butdrop()failed when quotes were provided, creating inconsistent behavior across DataFrame methods.User facing changes: users can now use either
drop('col')ordrop('"col"')consistently, matching the behavior of other DataFrame operations likeselect().Closes #1212
This is one of my first PRs, please let me know what I can improve!